[feat] Batch email notifications #276
Conversation
There was a problem hiding this comment.
@Dhanus3133 I have left some suggestions. I hope these solve your queries.
There was a problem hiding this comment.
@Dhanus3133 please start writin a short README section that explains how this feature works and add the new setting, remember to add a screenshot (push it to the docs branch).
@pandafy you know what would be handy?
- Allow to change the batching using an organization setting that allows user to change this per org via the admin
- Allow to specify whether the system should start batching right away instead of waiting 1 email, this could be disabled by default, and could also be available from the organization page
|
|
||
| # Check if the rest of the notifications are sent in a batch | ||
| self.assertEqual(len(mail.outbox), 2) | ||
| self.assertEqual(mail.outbox[1].subject, "Summary of Notifications") |
There was a problem hiding this comment.
I'd try to explore the possibility of using freezegun to simulate the passing of time and ensure that a new notification sent after the batch interval is not batched.
|
In our weekly call, @nemesifier and @pandafy suggested some improvements.
|
Dhanus3133
force-pushed
the
feat/batch-email
branch
from
7736e9d
to
4abc717
Compare
openwisp_notifications/settings.py
Outdated
| @@ -37,6 +37,10 @@ | |||
| 'openwisp-notifications/audio/notification_bell.mp3', | |||
| ) | |||
|
|
|||
| OPENWISP_NOTIFICATIONS_EMAIL_BATCH_INTERVAL = getattr( | |||
| settings, 'EMAIL_BATCH_INTERVAL', 30 * 60 # 30 minutes | |||
There was a problem hiding this comment.
Thinking out loud
I think 30 minutes is a lot of time. It will affect the real-timeness of the email notifications. Most of the consecutive notifications would be delayed by this time. We should also point this out in the README.
@nemesifier what do you think?
There was a problem hiding this comment.
Immediate email after batch email
If a notification is created just after the batch email is processed, then the email for this notification is sent immediately after the batch email.
Is this a desired behaviour? Can you please add a test for this?
Duplicate subject in email
The send_email function adds a subject in the email which servers as a heading for all the email communication.
I think, we should keep using the subject provided by the send_email function for consistency.
There was a problem hiding this comment.
@Dhanus3133 the test is failing because the notification email contains direct URL to the target object while the test expects redirect url
-<div class="msg"><p>Default notification with default verb and level info by <a href="https://example.com/api/v1/notifications/notification/2632e308-0f2d-43a1-8617-7350e48b8fd3/redirect/">Tester Tester (test org)</a></p></div>
+<div class="msg"><p>Default notification with default verb and level info by <a href="https://example.com/admin/openwisp_users/organizationuser/dea5c3cc-8c2a-4021-94a7-9cc36885f9ff/change/">Tester Tester (test org)</a></p></div>
| self.assertIn('Summary of 10 Notifications', mail.outbox[1].subject) | ||
| self.assertIn('View all Notifications', mail.outbox[1].body) | ||
| self.assertNotIn('Test Notification', mail.outbox[1].body) | ||
|
|
There was a problem hiding this comment.
Can we add another test in which this feature is disabled and ensure that no batching happens in that case?
Using the override_settings from django or mocking the setting should work.
There was a problem hiding this comment.
One thing I had asked before, but I am not sure I put in the review, I think we should add a way to open the web UI of openwisp and trigger the opening of the notification widget automatically, so that we can use that in the email: after clicking on the button to see all the remaining notifications, the user is taken to the application and the notification widget opens automatically to show the notifications.
Fixes #132